[fix](iceberg) Fix MVCC and nested schema evolution edge cases - #66345
Open
Gabriel39 wants to merge 18 commits into
Open
[fix](iceberg) Fix MVCC and nested schema evolution edge cases#66345Gabriel39 wants to merge 18 commits into
Gabriel39 wants to merge 18 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29212 ms |
Contributor
TPC-DS: Total hot run time: 169738 ms |
Contributor
ClickBench: Total hot run time: 23.86 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 28597 ms |
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-DS: Total hot run time: 168981 ms |
Gabriel39
marked this pull request as ready for review
August 1, 2026 05:48
Gabriel39
requested review from
924060929,
englefly,
morrySnow,
starocean999 and
yiguolei
as code owners
August 1, 2026 05:48
Contributor
ClickBench: Total hot run time: 23.99 s |
Gabriel39
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: three issues remain.
Checkpoint conclusions:
- Goal and proof: the PR addresses nested schema evolution, Iceberg MVCC/write binding, strict-cast struct nullability, and SQL comment quoting, but it does not fully achieve that goal because valid Iceberg write shapes and strict narrowing casts still fail, and the BE collection path adds a redundant scan. The added tests do not exercise those three cases.
- Scope and clarity: the changes are generally localized and the full 23-file diff was reviewed; no unrelated source change was identified.
- Concurrency and lifecycle: the resolved-empty snapshot marker, handle copies, scan initialization, synchronous/streaming/count paths, and RowDelta base validation were traced. Production pin ordering preserves the empty snapshot, and no lock-order, static-lifetime, or release issue was found.
- Configuration, compatibility, and persistence: no new configuration, storage-format, journal, symbol, or rolling-upgrade compatibility issue was found.
- Parallel paths and data writes: ARRAY/MAP, struct/named_struct, partial/static/full/rewrite/row-level writes, and both SQL backslash modes were checked. The Iceberg validator incorrectly treats a write subset as a full captured schema.
- FE/BE contract: snapshot state propagation is complete, but strict-cast logical nullability disagrees with the BE physical column type for narrowing casts.
- Performance: the new nested collection probe performs an avoidable O(entries) null-map pass for absent/all-clear inherited masks. No other material CPU, memory, or observability issue was substantiated.
- Tests and results: changed unit tests and assertions were statically inspected; no generated result-file issue was found. Builds and tests were not run because the authoritative review instructions prohibit them.
No additional user review focus was provided; the whole PR was reviewed.
| // only when a required descendant can consume them. This avoids scratch proportional | ||
| // to all array entries for the common all-required schema. | ||
| const NullMap* descendant_parent_null_map_ptr = nullptr; | ||
| if (_requires_parent_null_map_for_alignment(nested_column, |
Contributor
There was a problem hiding this comment.
_requires_parent_null_map_for_alignment() runs before this code establishes that the inherited parent mask exists and hides at least one row. For a large ARRAY/MAP with nullable-to-required descendants and a null or all-clear parent mask, the probe scans descendant null maps via has_null(), projection returns no entry mask, and normal alignment scans those descendants again. Please gate the descendant probe on a parent mask that can actually hide rows so this schema-evolution path does not add an avoidable O(number of entries) pass per block.
Contributor
There was a problem hiding this comment.
[P1] The new gate still projects when a hidden row owns no entries
The current head fixes the null/all-clear parent-mask case, but a set row-domain bit still does not mean the mask can hide an entry. For parent mask [1, 0] and ARRAY offsets [0, N], the only hidden row has an empty array. If the visible row contains a large nullable STRUCT element stream with one required-child physical NULL already covered by the element's own null bit, this preflight scans those N entries, _project_collection_parent_null_map() allocates and zero-fills an N-entry mask solely because row 0 is hidden, and the nullable-element recursion allocates a second N-entry union with that all-zero mask. MAP values have the same shape. Please first establish that a hidden row covers a non-empty [begin,end) offset range before scanning descendants/projecting, and add variable-cardinality ARRAY/MAP coverage for an empty hidden row plus a large visible collection.
| List<ConnectorColumn> boundColumns = handle.getColumns(); | ||
| boolean hasSyntheticRowId = boundColumns.size() == currentColumns.size() + 1 | ||
| && DORIS_ICEBERG_ROWID_COL.equals(boundColumns.get(boundColumns.size() - 1).getName()); | ||
| if (boundColumns.size() != currentColumns.size() && !hasSyntheticRowId) { |
Contributor
There was a problem hiding this comment.
handle.getColumns() is the query's bound write list, not the full schema snapshot. BindSink intentionally omits unspecified columns and static partition columns, and PhysicalPlanTranslator copies that subset into the handle, so a valid INSERT INTO t (id) ... for a two-column table (or an INSERT with a static partition) reaches this branch with different sizes and is rejected as schema drift even when the table never changed. Please carry the complete bound target schema separately for this comparison, while retaining the subset for value-to-column mapping.
|
|
||
| /** Nullability of this cast when invalid input fails instead of producing NULL. */ | ||
| public boolean strictModeNullable() { | ||
| return child().nullable(); |
Contributor
There was a problem hiding this comment.
Strict failure semantics do not make every successful cast physically non-nullable. BE's strict BIGINT-to-INT specialization still has type Nullable(Int32) (with an all-clear map for valid rows), while this reports a required field and FE serializes Struct<Int32>. The generic nested-type compatibility check admits that mismatch, then FunctionStruct tries to insert the ColumnNullable argument into a non-null ColumnInt32 field and hits the column-type assertion even for an in-range value. Please keep the field nullable until strict casts actually return non-null types, or normalize the strict cast's physical result as part of this change.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 28860 ms |
Contributor
TPC-DS: Total hot run time: 169312 ms |
Contributor
TPC-DS: Total hot run time: 165548 ms |
Contributor
ClickBench: Total hot run time: 23.82 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Issue Number: close #xxx Related PR: apache#66007 Problem Summary: Several review follow-ups from apache#66007 remained open: empty Iceberg reads could lose their MVCC boundary before a concurrent first append; nested DESCRIBE comments were not emitted as valid SQL literals; strict pre-casts widened required struct fields to nullable; trivial collection children lost inherited parent masks; and connector sink binding repeatedly loaded the latest target schema. This change preserves the empty-read marker through Iceberg write planning, centralizes SQL string literal quoting, makes cast nullability strict-mode aware, projects inherited collection masks through ARRAY/MAP offsets, and captures one case-insensitive connector target-schema snapshot per bind. Fix Iceberg MVCC and nested schema-evolution edge cases in connector reads, writes, DESCRIBE output, casts, and sink binding. - Test - [x] Regression tests - [x] Unit tests - Behavior changed: - [x] Yes. Correctness fixes for the affected edge cases. - Does this need documentation? - [x] No.
Keep an explicitly empty MVCC read empty across a concurrent first append and validate RowDelta conflicts from table creation. Add a barrier test for the MERGE/INSERT race. Also fail writes when the bound schema changed, avoid unnecessary nested collection null-mask allocation, and keep Cast nullability independent of thread-local strict mode.
Issue Number: None
Problem Summary: A query-begin pin for an empty Iceberg table retained only the negative snapshot id. Non-range partition specs could therefore fall back to live LIST metadata after a concurrent first append, while metadata-only partition-spec evolution could change the pinned view's range eligibility. In addition, the write-generation identity omitted the delete, update, and merge mode properties, allowing a property-only switch to copy-on-write after row-level DML admission. Preserve the empty generation's partition style in the pin, prevent live LIST fallback for resolved-empty handles, use pinned handles for other LIST fallback reads, and include all three row-level mode contracts in the write-generation fence.
None
- Test: Unit Test
- Ran focused Iceberg connector and FE MVCC unit tests.
- Ran connector forbidden-import and FE metadata-funnel validation gates.
- Ran full FE clean checkstyle with zero violations.
- Behavior changed: Yes. Resolved-empty Iceberg reads keep partition metadata at the query-begin generation, and row-level DML retries after a concurrent write-mode change.
- Does this need documentation: No
Gabriel39
force-pushed
the
fix/pr-66007-review-followups
branch
from
August 5, 2026 02:59
6254918 to
7306b4b
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
This PR fixes correctness and memory-usage edge cases in Iceberg MVCC writes and nested schema evolution:
ColumnNullable.The fix preserves the explicit empty-snapshot marker through scan planning and keeps RowDelta conflict validation anchored at table creation. In the concurrent empty-table MERGE/INSERT window, MERGE keeps reading the original empty view and its commit conflicts with the concurrent first append instead of producing duplicate data.
Write planning now carries an immutable complete target-schema snapshot separately from the write subset, validates column order, type, and Iceberg field id, and excludes engine-generated row-lineage metadata from the user-schema comparison. Nested collection alignment gates descendant scans on an inherited mask that can actually hide a row, and struct constructors preserve the physical nullability of cast results.
Release note
Fix Iceberg empty-snapshot concurrency, partial-write schema validation, nested collection memory usage, cast-result nullability, and nested DESCRIBE comment rendering.
Check List (For Author)
Tests
TableReaderTest: 95 passedgit diff --check: passed